Sanitize control/format characters in console logger output across all formatters#128741
Sanitize control/format characters in console logger output across all formatters#128741Copilot wants to merge 15 commits into
Conversation
|
Tagging subscribers to this area: @dotnet/area-extensions-logging |
Co-authored-by: rosebyte <14963300+rosebyte@users.noreply.github.com>
tarekgh
left a comment
There was a problem hiding this comment.
Review of the control character sanitization changes
The security motivation here is solid. Preventing terminal escape injection (ANSI sequences, bidi overrides, etc.) is worth doing. But the current implementation has several problems that need to be addressed before merging.
\n, \r, and \t should not be escaped
The sanitizer uses UnicodeCategory.Control which catches every character in U+0000-U+001F, including \n, \r, and \t. These are not security threats. They are structural formatting characters that the formatters depend on.
Both SimpleConsoleFormatter and SystemdConsoleFormatter have explicit downstream logic that operates on real newlines:
- SimpleConsoleFormatter.WriteMessage calls
message.Replace(Environment.NewLine, _newLineWithMessagePadding)to add indentation padding after each newline in exception text. - SystemdConsoleFormatter.WriteReplacingNewLine calls
message.Replace(Environment.NewLine, " ")to flatten multi-line messages into a single line (required by systemd/journald).
Because the sanitizer runs before these calls, it converts \n to the literal text \u000A. The downstream Replace calls then find no real newlines and become no-ops. This breaks multi-line exception formatting in Simple mode (no padding) and breaks the single-line guarantee in Systemd mode.
The fix should target only the actually dangerous characters: ESC (\x1B), BEL (\x07), backspace (\x08), bidi overrides (\u202E, \u202D), and similar. Not \n/\r/\t.
Double-escaping on the JSON path
Utf8JsonWriter with JavaScriptEncoder.Default already escapes all control characters (U+0000-U+001F) and all non-BasicLatin characters (including \u202E). Pre-sanitizing the strings is redundant and produces double-escaped output.
For example, ESC (\x1B) would normally appear as \u001B in the JSON output. With the sanitizer, it becomes \\u001B, which is a literal backslash followed by u001B. JSON consumers parsing these logs would see the text \u001B instead of the actual ESC character. The test changes in JsonConsoleFormatterTests.cs confirm this: they switched to expecting \\\\u000D\\\\u000A.
The sanitizer should be skipped entirely for the JsonConsoleFormatter path, or at minimum should not run when Utf8JsonWriter is handling the escaping.
Breaking change with default true
Setting SanitizeControlCharacters = true by default changes the output format for every existing application without any opt-in. Exception stack traces go from properly formatted multi-line output to a single blob containing \u000A literals. This will break log parsing tools and dashboards that expect the current format.
Consider either defaulting to false or narrowing the escape set so that \n/\r/\t pass through unchanged (which would make the default safe).
Minor issues
- API review: adding a public property to
ConsoleFormatterOptionsrequires going through the dotnet/runtime API review process. - Allocations: every exception log triggers a
StringBuilderallocation since exception strings always contain\n. Considerstring.CreateorValueStringBuilderfor the hot path. - Test coverage:
Log_ControlCharacters_SanitizationCanBeDisabledonly tests Simple and Systemd formatters. JSON opt-out is not covered. - Existing test expectations modified: the changes to
ConsoleLoggerTest.csnormalize broken formatting as the new expected output rather than preserving the original behavior.
|
A note on JSON output and custom encoders:
However, dotnet's JSON writer supports pluggable encoders, and one of them, Why we should leave it as it for now: • The encoder's name literally contains "Unsafe", so using it is a deliberate opt-in to relaxed behaviour. If this proves to be a real-world concern, we can add targeted sanitisation to the JSON path later without any API change. |
There was a problem hiding this comment.
Pull request overview
This PR introduces a shared sanitizer (ConsoleControlCharacterSanitizer) and applies it to the Simple and Systemd console formatter pipelines to reduce the risk of untrusted control / formatting characters influencing terminal output. It also adds new unit tests intended to validate sanitization behavior for non-JSON formatters.
Changes:
- Added
ConsoleControlCharacterSanitizerand used it to sanitize message / exception / category / scope text inSimpleConsoleFormatterandSystemdConsoleFormatter. - Updated
JsonConsoleFormatter’scharstate-property handling (but Json formatter still does not apply the new sanitizer). - Added tests for sanitization behavior (currently for non-JSON formatters only) and updated minor test code comments.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/Microsoft.Extensions.Logging.Console/tests/Microsoft.Extensions.Logging.Console.Tests/ConsoleLoggerTest.cs | Removes Arrange/Act/Assert comments in an existing test. |
| src/libraries/Microsoft.Extensions.Logging.Console/tests/Microsoft.Extensions.Logging.Console.Tests/ConsoleFormatterTests.cs | Adds new sanitization tests (currently limited to non-JSON formatters). |
| src/libraries/Microsoft.Extensions.Logging.Console/src/SystemdConsoleFormatter.cs | Sanitizes message/exception/category and scope string output prior to writing. |
| src/libraries/Microsoft.Extensions.Logging.Console/src/SimpleConsoleFormatter.cs | Sanitizes message/exception/category and scope string output prior to writing. |
| src/libraries/Microsoft.Extensions.Logging.Console/src/Microsoft.Extensions.Logging.Console.csproj | Adds ValueStringBuilder to support the sanitizer implementation. |
| src/libraries/Microsoft.Extensions.Logging.Console/src/JsonConsoleFormatter.cs | Adjusts JSON state-property writing for char values (allocates) but does not add sanitizer usage. |
| src/libraries/Microsoft.Extensions.Logging.Console/src/ConsoleControlCharacterSanitizer.cs | New sanitizer that escapes a hardcoded set of characters to \\uXXXX. |
|
Are we sure that Console Logger is right place to do such sanitization? I think it will negatively impact performance, even in many case when static constant strings are passed and sanitization will happen every time they are logged which seems like big overhead considering MECFG scale. I did not read thread model or any related documents, but to me it seems that this should be responsibility of caller. |
@BrennanConroy, @halter73, do you have any thoughts? Honestly, the argument users are supposed to sanitise the log message because they know the context, makes complete sense. We can still provide a convenience method to help them. |
| >= '\u200B' and <= '\u200F' => true, // zero-width and directional marks | ||
| >= '\u202A' and <= '\u202E' => true, // bidi embedding/override | ||
| >= '\u2066' and <= '\u2069' => true, // bidi isolates | ||
| _ => false, |
There was a problem hiding this comment.
Are there no problematic code points outside the BMP (i.e. those that would be composed of two chars)?
There was a problem hiding this comment.
Systemd and OpenSSH only remove C0+DEL+C1, how about following them?
|
The logger implementation (Console) knows what safe and unsafe input for its output is, the logging callers (app code) does not know what is safe or unsafe. |
|
Following up on the performance question raised above. Independent of where sanitization ultimately lives, if it stays in the Console logger it runs on the hot path for every The same escape set can be expressed as a static
(Local numbers, not CI hardware, so treat the absolute values as indicative.) The cost is pure CPU per log call, multiplied across the four sanitized values. Suggestions:
This keeps the point that the logger is the right place to know what is unsafe for its output, while removing the per-call overhead. |
…ControlCharacterSanitizer.cs Co-authored-by: Jiri Cincura ↹ <jiri@cincura.net>
|
So here is what we pay for the sanitisation in terms of performance:
Of course, the most important scenario is Clean, where there are no undesirable characters. Moreover, this only affects the Simple and Systemd formatters (JSON has its own escaping) for console logging, where we can reasonably expect two things:
One option we could consider is providing a version of the formatter without sanitisation as a non-default alternative, perhaps called something like UnsafeFormatter, to prevent users from repeatedly copying our old code. That said, I'd wait before doing that. We can reasonably expect that most users rely on console logging primarily for testing, debugging, and local development, where the performance difference is unlikely to be a deal-breaker. One adjustment we can do is dropping bidi overrides, zero-width, BOM, soft-hyphen and the other scattered Cf chars are not terminal-control vectors. The reason isn't performance but industry practice as both OpenSSH and systemd basically filter C0+DEL+C1 sets (systemd escapes \r since it's unix focused). |
I don't agree with that statement...
...but I do agree with that - apps where console logging is used as primary mechanism the performance difference is not going to be a problem. |
|
@tarekgh, thank you for the idea, implemented, the perf results I posted above already use it. |
| private static SearchValues<char> CreateSearchValues() | ||
| { | ||
| var chars = new List<char>(); | ||
| for (int c = char.MinValue; c <= char.MaxValue; c++) | ||
| { | ||
| if (ShouldEscape((char)c)) | ||
| { | ||
| chars.Add((char)c); | ||
| } | ||
| } | ||
|
|
||
| return SearchValues.Create(chars.ToArray()); | ||
| } |
There was a problem hiding this comment.
This might be eliminated if we narrow the sanitised characters to C0+DEL+C1.
Fixes #128727
Console logging currently writes untrusted control characters verbatim, allowing terminal escape/control effects and ambiguous output. This change sanitizes control/format characters across
SimpleandSystemdformatter paths.Cc/Cfcharacters as\uXXXXbefore writing log output.